Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(styles): Adjust heading styles #43640

Merged
merged 2 commits into from
Apr 8, 2024
Merged

fix(styles): Adjust heading styles #43640

merged 2 commits into from
Apr 8, 2024

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Feb 18, 2024 •
edited
Loading

Summary

Make headings look like real headings and not just ordinary text.
The styles are copied from the text-app where a design review was done on them.

Screenshots

before after
Screenshot 2024-02-18 at 13-04-13 Overview - Administration settings - Nextcloud Screenshot 2024-02-18 at 13-03-09 Overview - Administration settings - Nextcloud

Code used:

<h2>This is a h2</h2>
  <p>This is a paragraph</p>
<h3>This is a h3</h3>
  <p>This is a paragraph</p>
<h4>This is a h4</h4>
  <p>This is a paragraph</p>
<h5>This is a h5</h5>
  <p>This is a paragraph</p>
<h6>This is a h6</h6>
  <p>This is a paragraph</p>

Checklist

@susnux susnux added design Design, UI, UX, etc. 3. to review Waiting for reviews labels Feb 18, 2024
@susnux susnux added this to the Nextcloud 29 milestone Feb 18, 2024
ChristophWurst
ChristophWurst reviewed Feb 19, 2024
View reviewed changes
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

Settings are a bit inconsistent because those rendered with Vue are using NcSetttingsSection and it has font-size: 20px for .settings-section__name.

There are also a few headings that should be demoted to simple text, like here for external storage:

image

Not sure if we want to do this here or in a follow-up. Settings probably need a bit of a design review after this change.

@szaimen
Copy link
Contributor

szaimen commented Feb 19, 2024

Not sure if we want to do this here or in a follow-up. Settings probably need a bit of a design review after this change.

I would do it in a follow-up

szaimen
szaimen approved these changes Feb 19, 2024
View reviewed changes
Copy link
Contributor

@szaimen szaimen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshots look great :)

@ChristophWurst ChristophWurst added 4. to release Ready to be released and/or waiting for tests to finish 3. to review Waiting for reviews and removed 3. to review Waiting for reviews 4. to release Ready to be released and/or waiting for tests to finish labels Feb 19, 2024
@ChristophWurst ChristophWurst mentioned this pull request Feb 19, 2024
Merged
3 tasks
@ChristophWurst
Copy link
Member

Apps that use NcAppNavigationCaption

Before After
Bildschirmfoto vom 2024-02-19 15-33-00 Bildschirmfoto vom 2024-02-19 15-32-28

@susnux
Copy link
Contributor Author

susnux commented Feb 19, 2024 •
edited
Loading

If we agree on this changes here we should adjust nextcloud-vue styles accordingly.
Changes on the library will not conflict with older nextcloud versions but the fix it for 29 (this change here)

@susnux susnux modified the milestones: Nextcloud 29, Nextcloud 30 Feb 20, 2024
@skjnldsv skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Feb 23, 2024
@susnux
Copy link
Contributor Author

susnux commented Mar 29, 2024

@jancborchardt do we want to use this styles or adjust them because of nextcloud/text#5515 ?

@mejo- mejo- mentioned this pull request Apr 2, 2024
Merged
2 tasks
ChristophWurst
ChristophWurst approved these changes Apr 3, 2024
View reviewed changes
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's showtime

@ChristophWurst ChristophWurst added the pending documentation This pull request needs an associated documentation update label Apr 3, 2024
@ChristophWurst
Copy link
Member

Let's add this to the app dev upgrade docs as "breaking" frontend change

jancborchardt
jancborchardt approved these changes Apr 3, 2024
View reviewed changes
Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good @susnux but maybe as a follow-up it would make sense to adjust it to the sizes in https://github.com/nextcloud/text/pull/5594/files ? Maybe from h1 → h2 though because h2 is the first visible one (h1 is always the logo).

@susnux
fix(styles): Adjust heading styles
af770d6 
Make headings look like real headings and not just ordinary text.
The stylings are copied from the text app where a design review was done on them.

Signed-off-by: Ferdinand Thiessen <[email protected]>
@susnux
chore: Compile assets
ff11956 
Signed-off-by: Ferdinand Thiessen <[email protected]>
@susnux
Copy link
Contributor Author

susnux commented Apr 8, 2024 •
edited
Loading

But maybe as a follow-up it would make sense to adjust it to the sizes in https://github.com/nextcloud/text/pull/5594/files ?

Sizes adjusted ✅

Maybe from h1 → h2 though because h2 is the first visible one (h1 is always the logo).

Done ✅
But the latter is not always true. h1 is not the logo but for apps the app name (e.g. "Dashboard") or similar (for files it is the view (e.g. "All files").

@susnux susnux merged commit 1d5d8d4 into master Apr 8, 2024
105 checks passed
@susnux susnux deleted the fix/headings-styling branch April 8, 2024 14:05
@susnux susnux mentioned this pull request Apr 9, 2024
Merged
@Pytal Pytal mentioned this pull request May 9, 2024
Merged
3 tasks
@blizzz blizzz mentioned this pull request Jul 24, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@szaimen szaimen szaimen approved these changes

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@jancborchardt jancborchardt jancborchardt approved these changes

@nimishavijay nimishavijay Awaiting requested review from nimishavijay

Assignees
No one assigned
Labels
2. developing Work in progress design Design, UI, UX, etc. pending documentation This pull request needs an associated documentation update
Projects
None yet
Milestone
Nextcloud 30
Development

Successfully merging this pull request may close these issues.

[Bug]: h3 to h6 look like text, headings generally inconsistent across apps
5 participants
@susnux @szaimen @ChristophWurst @jancborchardt @skjnldsv